Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule S6423: Always log failures in Azure Functions #5645

Merged
merged 107 commits into from
May 31, 2022

Conversation

martin-strecker-sonarsource
Copy link
Contributor

@martin-strecker-sonarsource martin-strecker-sonarsource commented May 9, 2022

Fixes #5599

  • Supports Az Function .net 4.6 and adopt tests accordingly. Update: target of version 1.x -> Not supported.
  • ILogger as static field. Update: Only needed if catch block is outside of entry point. Update: instance field used in the DI samples
  • Is Ilogger optional in an Azure function? If yes, check the presence. Update: It's optional, so check the presence. Update: Done in 4876dcf
  • What about TraceWriter in version 1.x Update: target of version 1.x -> Not supported.
  • What about try/catch outside the annotated entry point
  • What about DI containers
  • Should we respect Disable?
  • Offer code-fix?
  • calls on custom extension methods (e.g. for the exception filter case)
  • ILogger via constructor injection https://docs.microsoft.com/en-us/azure/azure-functions/functions-dotnet-dependency-injection#iloggert-and-iloggerfactory
  • generic ILogger<T> (see example above)
  • What about telemetry client as an alternative way of logging?
  • What about other logging libraries (e.g. Serilog) Rule S6423: Always log failures in Azure Functions #5645 (comment)
  • The rule makes sense for every catch block and is not just related to AzureFunctions. Should we remove the AzureFunction requirement?
  • FP: A caught OperationCancelledException does not need to be logged
  • FP: A throw statement in a catch clause is delegating the logging to the next level. No need to log the exception twice.

Azure functions entry points: https://docs.microsoft.com/en-us/azure/azure-functions/functions-dotnet-class-library?tabs=v2%2Ccmd#methods-recognized-as-functions

@martin-strecker-sonarsource martin-strecker-sonarsource added this to the 8.40 milestone May 9, 2022
@martin-strecker-sonarsource martin-strecker-sonarsource marked this pull request as ready for review May 11, 2022 09:50
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 1 for scaffolding and UTs. I did not review implementation file yet.

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 2, rule implementation

@martin-strecker-sonarsource martin-strecker-sonarsource changed the title S6423: Azure Functions should log all failures Rule S6423: Always log failures in Azure Functions May 12, 2022
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 3 - for everything except implementation

@Corniel
Copy link
Contributor

Corniel commented May 16, 2022

It is a nice and ambitious rule. Consider support of at least Serilog as an alternative logging mechanism.

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 4, second iteration over implementation

@martin-strecker-sonarsource
Copy link
Contributor Author

@Corniel Thanks for pointing out other logging libraries. I added it to the to-do list in the PR description.

@martin-strecker-sonarsource martin-strecker-sonarsource force-pushed the Martin/AzFunction_LogFailures branch from b8115dc to c89ccad Compare May 30, 2022 08:45
Co-authored-by: Costin Zaharia <56015273+costin-zaharia-sonarsource@users.noreply.github.com>
@martin-strecker-sonarsource martin-strecker-sonarsource force-pushed the Martin/AzFunction_LogFailures branch from c89ccad to fdd54cf Compare May 30, 2022 08:50
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Polishing

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last round

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

91.0% 91.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! That was a big one

@pavel-mikula-sonarsource pavel-mikula-sonarsource merged commit 6b8ab5e into master May 31, 2022
@pavel-mikula-sonarsource pavel-mikula-sonarsource deleted the Martin/AzFunction_LogFailures branch May 31, 2022 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule S6423: Always log failures in Azure Functions
4 participants